fix(egosuite): validate label report envelope - #364

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0b943e7eb3

The validation itself is right and I would merge it as it stands. It needs a rebase, and the rebase is not mechanical, so here is what collides before you spend time on it.

#362 merged about an hour ago and rewrote the same loader: load_projected_hand_label_report now returns a ProjectedHandLabelReport and matches labels by HFlow source_uri instead of the basename. Merging current main into your branch auto-merges evaluate.py cleanly, conflicts only in the test file, and then fails five tests. Two different causes:

Four are mechanical. #362's new tests hand-write reports as {"frames": [...]} with no envelope, so your check refuses them before they reach what they are testing:

test_legacy_label_report_rejects_one_basename_for_multiple_sources
test_saved_label_report_matches_same_named_sources_by_canonical_provenance
test_saved_label_report_rejects_missing_or_unrelated_canonical_provenance[both cases]

  Actual message: field 'schema_version' has value None; supported value is 1

Adding "schema_version": 1, "label_type": "projected-hand-joints" to those four fixtures is the whole fix, and arguably your check is right to have caught them.

One needs a decision from you. You rewrote test_saved_label_report_selects_exact_frames_for_a_canonical_episode to build its report through write_label_report, which is DoD 6 and the better fixture. On main that test hand-writes a report with no source_uri, so it matches through #362's legacy basename path. Through write_label_report the report now carries a real source_uri, matching switches to the identity path, and the hardcoded {"source_uri": "run-a/episode-123.mcap"} no longer corresponds to what was recorded:

ValueError: label manifest has no frames for source identity 'run-a/episode-123.mcap'

The identity write_label_report records comes from App.source_identity() against HFLOW_DATA_ROOT, so for a tmp_path source it is an absolute path, not run-a/.... Reading the recorded source_uri back out of the written report and passing that is the robust answer, and it keeps your real-envelope fixture. Hardcoding a second identity would just move the coupling.

Nothing above is a criticism of the change. Excluding bool from the schema_version integer check is the detail I look for and most people miss: isinstance(True, int) is True, so without that line {"schema_version": true} would have been accepted as version 1. This repo has been bitten by that more than once.

One thing for next time, and it is the reason this is a request rather than a merge with a fixup from me: pytest -q reporting 11 skipped rather than 6 means it ran without --all-extras. uv sync --locked --all-extras is the documented form, and the difference is the mediapipe and vision tests.

Last thing, an invitation rather than a rule. You have eleven merged PRs here, so the good first issue pool is best left for people arriving after you. The higher-leverage work is the advanced backlog, and #365 was filed today by another contributor from actually running the thing against Egocentric-10K, which is the kind of issue worth more than any single starter fix. #311, #312, #320 and the bucket-backed set (#303, #304, #305) are all open and unassigned.

LGTM, merging. The rebase landed the way it needed to and the envelope check is properly held.

{**envelope, "frames": [None]} is what makes the test name true. A frame list that cannot parse means the refusal can only be coming from the envelope, so "before frames" is an assertion rather than a description. Each of the seven cases is the only failure when its own clause goes:

schema_version check removed   4 cases fail, no others
label_type check removed       3 cases fail, no others

Reading the recorded source_uri back out of the report instead of hardcoding run-a/episode-123.mcap is the better half of the change. It ties the writer's identity to the reader's lookup, and it fails now if the two drift: repointing write_label_report at a different label type breaks the exact-frame test.

I pushed one fixup, de57a4d. The bool guard was the one clause nothing held. bool subclasses int, so True satisfies isinstance(_, int) and True != 1 is False, which means removing that clause left all 54 tests green while {"schema_version": true} sailed through. You called it out by name in the rebase notes, so it was worth a case of its own rather than a comment.

One thing left as is: the isinstance(label_type, str) half of the label_type check is unreachable, since only a str can equal the constant. Dropping it changes no test. Harmless, and it reads as intent, so I left it.

Gate on the merged result. Example project: ruff check, ruff format --check, ty check clean, 55 passed. Root suite: clean, 1489 passed / 6 skipped. Your 17 macOS failures were the native-overlay tests from 07dbc69; they pass here on Linux, so your read on them was right.

Fixes #307. One ask, and it is an invitation rather than a rule: the good first issue label is there to give newcomers somewhere to start, and with eleven PRs you are well past needing it. Please leave that pool alone and take from the advanced list instead: https://github.com/Hebbian-Robotics/hflow/issues?q=is%3Aissue+is%3Aopen+label%3Aadvanced

Given your transform and video history in particular, the thing most worth your time is not an issue we wrote. Point HFlow at a real corpus (Egocentric-10K or Egocentric-100K on Hugging Face) and tell us what breaks, what is slow, or what is awkward. #287 came out of exactly that and it has been more useful than anything we would have thought to file.